-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance xenorchestra_vm
to provide a method for graceful termination
#222
base: master
Are you sure you want to change the base?
Conversation
This allows the VM to terminate its work properly.
As discussed in #212 Trying to stop a VM that is not running results in an error, so we only do shutdowns for running VMs now Possible power-states are "Running, Halted, Paused, Suspended"
Add acceptance tests that verify if graceful termination was successful
ad1b03f
to
5d9cfbf
Compare
@4censord please review this to verify it will work for your use cause. The only functional difference should be that this functionality must be opted into with a new After discovering #220, setting this to the default has the potential to significantly slow down the tests and the general case. From running the test suite a few times against #212, the default 2 min timeout for validating PV drivers are present causes more flakiness in the build and longer test times. I think it should be expected that if terraform is to destroy resources that it may not be graceful. Therefore, I think making this opt in is the best solution. |
This would work for me. I would still argue that it should be the default behavior.
Most other providers seem to work this way. But i understand that it is not practical to do so right now When attempting to shut down a vm without the management agent installed, this now times out.
|
Changing the
|
It seems vsphere does this, but uses a 3 min timeout by default. What other providers have you seen?
#220 may need a slightly different heuristic. I'll see if the XO api exposes the use of PV drivers rather than relying on the management agent presence. I think we should handle the case were there aren't PV drivers potentially. Allowing people to opt into a forceful stop and using that if the graceful method fails or timeouts could satisfy that, but I wanted to see how XO handles things.
Modifying Vms unfortunately relies on a 25 second sleep (source). That's another area where there is a race condition and not being able to detect the value has persisted causes problems for the test suite. I don't think I can fix that in this PR, but I can look into that early next year. |
Does or attempts graceful termination:
Does not do graceful termination:
Oh, i see. |
#223 will address the regression with requiring the management agent and will properly detect PV drivers |
I think I'd like to take another pass at this and implement things closer to how hyperv works:
Having forceful shutdown should prevent the test suite flakiness that I experienced when setting the graceful shutdown as the default. So hopefully that should be more in line what you initially were advocating for. |
This is a continuation of #212.
I discovered that #220 needed to be addressed to avoid a race condition that existed when the XO client inside the provide tried to shutdown Vms. In addition to this, I decided that this was better to provide as an opt in rather than the default behavior.
Testing
make testacc
passesxenorchestra_vm
delete behavior